Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow functions to be specified for scale grid line options #6700

Merged
merged 7 commits into from
Nov 12, 2019

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Nov 6, 2019

Builds off of #6697 to add function options for grid line styling. I couldn't support a function for the borderDash option because the type is an array and so indexing would go badly.

I added a sample, the image of which is shown below

scriptable-scale

@benmccann
Copy link
Contributor

Just FYI, the build is failing

@etimberg
Copy link
Member Author

etimberg commented Nov 6, 2019

Looks like I have a lint error in the HTML sample

src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a stupid idea, but should we consider making grid lines Elements?

src/core/core.scale.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

Might be a stupid idea, but should we consider making grid lines Elements?

I'm trying to do an Element refactoring at the moment, so would prefer to hold off on anything along those lines to avoid complicating it. Hope to have something to share shortly!

@etimberg etimberg force-pushed the scriptable-scale-options branch 2 times, most recently from bbb8a99 to 5cc283c Compare November 7, 2019 11:33
@kurkle
Copy link
Member

kurkle commented Nov 7, 2019

Note: build is failing

@etimberg
Copy link
Member Author

etimberg commented Nov 7, 2019

Thanks @kurkle. Looks like I rebased something badly

@benmccann
Copy link
Contributor

The docs don't specify what the function context is which might be useful since it's not the normal context that's passed in.

I couldn't support a function for the borderDash option because the type is an array and so indexing would go badly.

Could you just make this one scriptable and not indexable? (though even with indexing it's not quite clear to me what would be wrong. the user could theoretically pass an array of arrays)

@kurkle
Copy link
Member

kurkle commented Nov 8, 2019

@benmccann resolve will think the normal single value is indexed, because it is an array. So would need some changes to resolve or how borderDash is configured.

Not quite in the scope of this PR:
I think it would actually make sense to have a border option object, that can be indexable / scriptable and having color, dash, width, dashOffset properties in that.

Another thing I was thinking about is hover, point etc, rather than prefixing the key, we could include a object instead:

left cfg = {
  type: 'line',
  data: {
    datasets: [{
      border: {
        width: 2
      },
      point: {
        backgroundColor: 'red'
      },
      data: [1, 2, 3]
}

This would be easier in some aspects. Could be also faster in some cases (define all border properties in a single function call vs 4 different calls when scriptable).
Could also be a nightmare to memory usage / non scripted config resolution, not quite sure.

kurkle
kurkle previously approved these changes Nov 8, 2019
@benmccann
Copy link
Contributor

@etimberg looks like this one will need to be rebased

kurkle
kurkle previously approved these changes Nov 10, 2019
@benmccann
Copy link
Contributor

Lgtm. The only comment I had was that the docs don't specify what the function context is. I think that might be useful to include since it's not the normal scriptable options context that's passed in.

@etimberg
Copy link
Member Author

@benmccann I added a note on the context format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants